Also, I wonder if maybe the WakerRegistrar thing would be better served by copying the kqueue design pattern whereby the Selector provides factory-type methods to create and use a waker. I didn't notice that it already is working pretty similarly to what I need until just now...

Indeed, I think I found a much cleaner way to detangle Waker behaviour from the Selector so that we can have the concept of a WakerDriver (a low-level description of how to call eventfd, pipe, pipe2, or kevent) vs a Waker which is created by the selector and introduces some customization of the WakerDriver behaviour based on the selector's quirks. For example, the poll selector can use either eventfd or pipe2 WakerDriver, but the Waker it creates requires reregistering the interest with poll before wake can be called. The epoll selector can use either eventfd or pipe2 WakerDriver, but the Waker it creates has a no-op before each call to wake.

This seems to elegantly resolve one existing gotcha and two that I created:

1. pipe::Waker::wake no longer needs an illumos-specific cfg check. Instead, the epoll selector will have that check since it is epoll-specific behaviour on that platform that's the issue.
2. I can drop my WakerRegistrar as I can now have the poll selector directly control reregistration of whatever WakerDriver is used
3. The poll selector won't need another eventfd/pipe impl living in side the poll selector for the purposes of internally waking a blocking poll call. We can just use the WakerDriver internally.

This also fixes the FreeBSD failing tests (which are actually just because WakerRegistrar is unused in the kqueue path)

I don't really have a lot of time to review a pr this big. Can you maybe remove the changes to the waker to reduce the size of the pr? The pipe implementation should work on any Unix right?

@Thomasdezeeuw some important context here is that I didn't change much from #1602 which was already pretty heavily reviewed and discussed. The main changes are mentioned in the PR summary, and all changes were done directly because of legitimately failing tests (i.e. unintended changes in behaviour).

While I admit I took an approach that moves around a lot more code than is needed, the reason I did this was two fold:

1. The way Waker and Selector interact with each other for poll vs epoll is pretty significant and this would require an ugly hack like this (and it would be copy/pasted for eventfd and pipe):

pub struct Waker {
  selector: Option<Selector>,
  ...
}
impl Waker {
  pub fn new(selector: &Selector, token: Token) -> io::Result<Self> {
    ...
    cfg_poll_selector! {
      Ok(Waker { selector: Some(selector.try_clone()?, token, fd: file })
    }
    cfg_epoll_selector! {
      Ok(Waker { selector: None, token, fd: file })
    }
  }
  pub fn wake(&self) -> io::Result<()> {
    cfg_poll_selector! {
      self.selector.unwrap().reregister(self.fd.as_raw_fd(), self.token, Interest::READABLE)?;
    }
    ...
  }

2. The poll-based selector needs an internal Waker (calling register needs to interrupt poll ofc), so this creates a tear where you can have a totally separate waker behaviour for when register is called vs when you use the public Waker API. For example the original PR used pipe with a blocking write-side fd for the internal register waker, but used eventfd for the Waker API. Dropping the refactor will mean that this inconsistency must remain and I'd argue this lowers confidence that the Linux-based tests are even a good proxy for the platforms we're trying to support. One of the main sticking points identified in Poll selector #1602 (comment)

Happy to proceed with whatever you recommend however, though if we drop the refactor I could use some help finding a cleaner way to do (1) or at least an understanding that it's acceptable to not do it cleanly :\

Also yes, in theory the poll+pipe implementation should work on any UNIX, I'm right now working with a FreeBSD VM to try to get that to build and work nicely but it currently does not.

Oh and also I could break up the refactor into a separate PR so it's easier to swallow and confirm that it's correct without introducing the poll backend.

I was wrong re my statement that any UNIX should be able to support the poll+pipe backend. While technically true that it will "work", it's hopeless to get all current tests to pass because libc::POLLRDHUP is Linux-specific and without it tests like tests/tcp.rs:write_shutdown will fail. There are about a dozen tests like this. This does mean that mio still works of course, just that shutdown(Read/Write) will not behave as expected and the caller will need to detect these shutdown events by trying to read or write.

Also while I'm waiting for review it looks like you can kick the CI tests so I can confirm everything is working correctly. Cirrus is running, but not GitHub Actions.

I wanted to review this in the weekend, but I didn't. I think this still has a lot of changes that are not strictly necessary for poll(2) support. From the changes to IoSourceState (can be contained to src/sys/unix/poll.rs), to the macros (not required), but also the waker (this should just be an pipe/eventfd fd with IoSourceState). Also unnecessary name changes Waker to WakerDriver?

Maybe I have time next weekend, but not promises, I do want to reduce the number of changes though. I really like to see a minimal set of changes to implement this.

From the changes to IoSourceState (can be contained to src/sys/unix/poll.rs)

Ackd, I'll avoid breaking things out into separate files.

...to the macros (not required)

I only did this because the original PR reviewer asked for it because the PR will require copy/pasting the cfg criteria quite a few times. I can drop it though, but see my comments below so we're on the same page as to how many times this will need to get repeated.

...but also the waker (this should just be an pipe/eventfd fd with IoSourceState)

I'm not sure how this can be achieved. The critical difference between epoll vs poll is that for poll you either need to read the pipe/eventfd when it wakes (what the original PR did) or you need to reregister the interests again before calling wake (what my PR did). In either case, there needs to be code that allows the poll Selector to interact with Waker internals. IoSourceState is also but separately interacting with Selector internals.

Fwiw the reason I opted for a different strategy was so that I didn't need to deepen the circular relationship between Selector and Waker but could rather describe the dependencies as such:

Selector -> Waker -> WakerDriver
Waker -> Selector

Also unnecessary name changes Waker to WakerDriver?

There wasn't a rename here as such, but rather WakerDriver was added to be used by Waker. This is because as I mentioned above in a previous comment, we need to use pipe or eventfd to internally wake poll() when register() is called. But Waker as written right now calls register in the constructor making it a bit fragile to reuse internally. So I stripped WakerDriver away which is just a pure side effect free API and then added Waker back inside the Selector with the register side effect. This allowed the poll Selector to internally use WakerDriver and this neither duplicated code nor, as the original PR did, unintentionally use pipe for the internal register() wakeup case but eventfd for the Waker case.

Maybe I have time next weekend, but not promises, I do want to reduce the number of changes though. I really like to see a minimal set of changes to implement this.

Understood, I'm willing to rework the PR with reduced churn, it's just not 100% clear where the tradeoffs should be. See my comment above about how clumsy it gets to try to conditionally call reregister() inside wake() only for the poll Selector case. If that clumsiness is preferred, that's what I'll do.

It is, I admit, less intrusive to use the original PR's strategy for reading the pipe/eventfd on wake tho since you only need to add new constructor APIs that don't do the register (e.g. new_internal), then add a new clear() API that resets them as needed. I'll play around with this approach some more and see if I can't get it to look a little less clumsy than the original PR had it.

Also wanted to thank you for taking the time to review and discuss. I know this poll support is probably weighing on you as a reviewer a bit after so much churn and iteration. This will be such a huge help for the esp32 community once we're able to get it over the line though!

...to the macros (not required)

I only did this because the original PR reviewer asked for it because the PR will require copy/pasting the cfg criteria quite a few times. I can drop it though, but see my comments below so we're on the same page as to how many times this will need to get repeated.

I'm personally a bit torn on the macros. For things like cfg_net they help, but for the selector I'm no so sure, I think the code/cfgs is easier to follow without th emacro.

...but also the waker (this should just be an pipe/eventfd fd with IoSourceState)

I'm not sure how this can be achieved. The critical difference between epoll vs poll is that for poll you either need to read the pipe/eventfd when it wakes (what the original PR did) or you need to reregister the interests again before calling wake (what my PR did). In either case, there needs to be code that allows the poll Selector to interact with Waker internals. IoSourceState is also but separately interacting with Selector internals.

I was thinking an eventfd/pipe who's fd is always passed to poll (assuming the waker was added), preferably in a fixed position in the pollfds. Then we always check the waker's pollfd and drain it's readiness if we get an event (e.g. read from the pipe). For the Waker we can't use the kqueue implementation as it has no fd, but the eventfd and pipe should continue working as normal, I would expect. The only change in the Waker code would be a special call to register, something like selector.register_waker, which for poll(2) would mean to duplicate the fd and use it as described above. For epoll we can just call register.register (as we currently do).

Fwiw the reason I opted for a different strategy was so that I didn't need to deepen the circular relationship between Selector and Waker but could rather describe the dependencies as such:

Selector -> Waker -> WakerDriver
Waker -> Selector

Also unnecessary name changes Waker to WakerDriver?

There wasn't a rename here as such, but rather WakerDriver was added to be used by Waker. This is because as I mentioned above in a previous comment, we need to use pipe or eventfd to internally wake poll() when register() is called. But Waker as written right now calls register in the constructor making it a bit fragile to reuse internally. So I stripped WakerDriver away which is just a pure side effect free API and then added Waker back inside the Selector with the register side effect. This allowed the poll Selector to internally use WakerDriver and this neither duplicated code nor, as the original PR did, unintentionally use pipe for the internal register() wakeup case but eventfd for the Waker case.

That makes more sense. If my idea describe above doesn't work we can go with something like this. But I'm not sure about the "driver" name, but we can bikeshed this later.

Maybe I have time next weekend, but not promises, I do want to reduce the number of changes though. I really like to see a minimal set of changes to implement this.

Understood, I'm willing to rework the PR with reduced churn, it's just not 100% clear where the tradeoffs should be. See my comment above about how clumsy it gets to try to conditionally call reregister() inside wake() only for the poll Selector case. If that clumsiness is preferred, that's what I'll do.

See above, but I prefer to limit the scope of this pr, affecting the minimal amount of non-poll(2) code would be ideal. That makes it easier to review and less risky to merge as other code isn't affected.

It is, I admit, less intrusive to use the original PR's strategy for reading the pipe/eventfd on wake tho since you only need to add new constructor APIs that don't do the register (e.g. new_internal), then add a new clear() API that resets them as needed. I'll play around with this approach some more and see if I can't get it to look a little less clumsy than the original PR had it.

Also wanted to thank you for taking the time to review and discuss. I know this poll support is probably weighing on you as a reviewer a bit after so much churn and iteration. This will be such a huge help for the esp32 community once we're able to get it over the line though!

I appreciate this. I very much want to help the esp32 community, but my main priority is to keep the existing code working and maintainable (for my own sanity).

I was thinking an eventfd/pipe who's fd is always passed to poll (assuming the waker was added), preferably in a fixed position in the pollfds. Then we always check the waker's pollfd and drain it's readiness if we get an event (e.g. read from the pipe). For the Waker we can't use the kqueue implementation as it has no fd, but the eventfd and pipe should continue working as normal, I would expect. The only change in the Waker code would be a special call to register, something like selector.register_waker, which for poll(2) would mean to duplicate the fd and use it as described above. For epoll we can just call register.register (as we currently do).

Looking into more detail here and this is basically how the original PR (#1602) worked but it needed unsafe handling of the waker RawFd and a change to all sys::Selector impls. I also realized that your suggestion to dup the fd won't work directly directly because esp32 doesn't implement dup but it can work indirectly by just internally re-using the waker we know the poll-impl already has:

#[cfg(mio_unsupported_force_poll_poll)]
mod poll {
  pub struct Waker {
    selector: sys::Selector,
  }

  impl Waker {
    pub fn new(selector: &Selector, token: Token) -> io::Result<Waker> {
      Ok(Waker { selector: selector.try_clone()? })
    }

    pub fn wake(&self) -> io::Result<()> {
      // Since this is the poll-only impl, the Selector would need only to call self.notify_waker.wake() and everything would
      // just work the exact same way it does when you call Selector::register.
      select.selector.wake_internal()
    }
  }
}

This would then save on an fd and make the code path far, far clearer how it's being used. So, the only real issue then is how can we get the poll impl to re-use the code we have in eventfd::Waker and pipe::Waker. I'd propose that once again the WakerDriver idea helps us here (pending a rename perhaps):

1. Expose pub(crate) versions of eventfd::Waker and pipe::Waker as a new type (for argument let's say WakerImpl) that doesn't interact with Selector at all (like what my PR did with WakerDriver). This can be a simple rename mostly, but unlike my PR today will not include other Waker implementations and is only meant to be an internal sys/unix utility to re-use eventfd/poll impl code.
2. Convert the eventfd::Waker and pipe::Waker types to be simple pass-throughs to eventfd::WakerImpl and pipe::WakerImpl with the extra register behaviour on top.
3. Add a new poll::Waker as written above that is a simple pass-through to poll::Selector::wake_internal.

Thoughts? I'm working on this locally now in parallel but I want to be respectful of your suggestions and not blindside you with changes to the PR that aren't quite like we discussed already.

Force pushed a reworked version of the PR that I think is much cleaner based on your feedback and on my comment above. Heads up that there is a functional change here to re-use the internal Waker which both simplifies the implementation and makes it much clearer why we had to slightly refactor eventfd::Waker and pipe::Waker.

Silly mistake had the CI checks not running, now that they're up and failing I'll go ahead and fix.

EDIT: I see now you have to manually run them each time I push commits. It is possible to relax this in the repo's Settings under Actions -> General, Require approval for first-time contributors who are new to GitHub. Up to you ofc if you want to enable this.

Applied most of the suggested changes (see comments above on the others) and confirmed tests still pass. If you prefer I can squash all commits to prepare to merge. Also once again, huge thanks for your time and patience on this PR. I know it's big and I really appreciate your attention to detail.

Ok applied a few more suggestions and left a tracking issue for the optimizations. Again, I can squash all these commits if you prefer, just lmk how you want to proceed to merge.

Just caught an issue from CI that needs to be addressed, working on fixing up a bad assumption that IoSourceState in sys/shell/mod could assume non-Windows was UNIX which breaks for wasm32-wasi

Thanks @jasta for pulling this over the finish line!

Thanks @jasta for pulling this over the finish line!

Don't you mean polling? Har har har :) But really a big thanks to you as well for taking the time and effort to make sure the crates you maintain continue to meet a high quality bar. The rust community is lucky to have you!